Skip to content

fix: set networkConfiguration to null when using bridge network mode #617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

nburt
Copy link
Contributor

@nburt nburt commented Aug 16, 2024

Issue #, if available:
#616

Description of changes:
This PR sets networkConfiguration to null when neither subnetIds nor securityGroupIds are set. This fixes run task whenever launch type is EC2. I tested this branch with the github workflow I'm building and it works to run migrations as a task.

There was one failing test which I changed. I wonder whether this should only apply when the launchType is EC2 though? Let me know what you think

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -48,10 +48,10 @@ async function runTask(ecs, clusterName, taskDefArn, waitForMinutes) {
awsvpcConfiguration["securityGroups"] = securityGroupIds.split(',')
}

if(assignPublicIP != ""){
if(assignPublicIP != "" && (subnetIds != "" || securityGroupIds != "")){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, we should call this out in the action.yml file. in the description of assignPublicIP field.

It would also be great if we could add a line describing the difference in networkconfiguration for different launch types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok great, I made a few readme + actions.yml description changes. let me know what you think!

@amazreech amazreech changed the title set networkConfiguration to null when using bridge network mode fix: set networkConfiguration to null when using bridge network mode Aug 16, 2024
@amazreech
Copy link
Contributor

Thank you for the PR!

I wonder whether this should only apply when the launchType is EC2 though? Let me know what you think

For this, I am thinking if it should be enough to call this out in the README. Mostly because:

For LaunchType Fargate the only network mode allowed seemed to be awsvpc. When the networking mode awsvpc is used, networkConfiguration seems to be mandatory. I tested this and saw that GitHub Action throws a error when networkConfiguration is passed as null with a launchtype Fargate:

image

Source: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html#:~:text=If%20using%20the%20Fargate%20launch,awsvpc%20modes%20can%20be%20used.

Copy link
Contributor

@amazreech amazreech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! LGTM!

@amazreech amazreech merged commit 0a1e247 into aws-actions:master Aug 19, 2024
6 checks passed
@nburt nburt deleted the fix-bridge-network-mode branch August 19, 2024 15:13
@nburt nburt restored the fix-bridge-network-mode branch August 19, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants